-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Conversation
6e311b4
to
9e87d64
Compare
@perdasilva Thanks for your contribution! @mxnet-label-bot add [pr-work-in-progress, test] |
@mxnet-label-bot add [pr-awaiting-review] |
@mxnet-label-bot remove [pr-work-in-progress] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution! Wanted a few changes/clarification. Rest looks good. Also, looks like the RAT license causes CI to fail for sanity job
@@ -250,6 +250,20 @@ function set_instruction_set() { | |||
${sorted_indexes[$end_buildfromsource_command_index]}) | |||
} | |||
|
|||
# given a build commands string, filter any build commands that ought not be executed | |||
# during the test. An example would be git clone'ing the mxnet repository. You want to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: cloning* and MXNet*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also would like to rephrase the sentence "You want to run the tests against the current commit being tested in Jenkins" - I am unable to understand how does in this example "filter_build_command" play a role?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll update the comment - the git clone'ing is to reference the git clone command. But fair comment anyway.
The issues is the build from source commands include cloning the repository, which checks out the master branch. The problem occurs when we are testing build from source commands pinned to v1.3.x against the master branch. This is why the nightly is failing.
The chosen approach to mitigate this was to filter out these git commands (and the cd into the incubator-mxnet directory). This means that the build from source commands will run directly on the version of the repository that is being tested by Jenkins and not against master.
does it make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. Now it makes sense. Thanks for the clarification.
@ChaiBapchya I've addressed your comments - please let me know if the issue is clearer now and the commend over the function more helpful |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM w/o spelling mistakes. Thanks for the PR!
# $ git clone --recursive https://github.com/apache/incubator-mxnet | ||
# $ cd incubator-mxnet | ||
# if these commands get executed in the jenkins job, we will be testing the build from source instructions | ||
# against the master branch and not against the version of the repository that Jenkins checksout for testing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: checks out
# This presents a particularly big problem for the version branches and their nightly builds. Because, | ||
# we would, in effect, be testing the build from source instructions for one version of MXNet against | ||
# the master branch. | ||
# in this function we target the commands cited in the example above, but leave it open for expantion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: "In" "explanation"
Please address the comments in a separate PR. Merging to unblock pipeline. |
Description
The nightly job was failing because the installation instructions test was testing v1.3.x instructions against the
master
branch. I've updated the underlying script to filter out the git calls from the set of operations it extracts from the instruction document.Targets #13800
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Fixes nightly job by filtering some commands when running the installation guide test.
Comments
This commit was cherry-picked to be merged to branch
v1.3.x
on PR #14145